Skip to content

Fix Memory leak in make loop #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 26, 2018
Merged

Conversation

safareli
Copy link
Contributor

Fixes #10
Simplifies make loop (attempt/traverse_ removed)

safareli added 3 commits May 21, 2018 10:12
if `var` is killed with an error, `takeVar var` will throw that same error so the loop will be terminated.

note: before purescript-contrib#6 attempt  was not used
@safareli
Copy link
Contributor Author

safareli commented May 22, 2018

I still had to use attempt, as without it the Done error is thrown I guess on next tick after call to Bus.kill (error "Done") bus. is this expected behavior of Aff @natefaubion ? I guess i shouldn't throw, as launchAff returns Fiber and i expect Fiber to throw once it's "joined".

Testing read/write/kill...
ok
/home/travis/build/slamdata/purescript-aff-bus/output/Control.Monad.Aff/foreign.js:513
                throw util.fromLeft(step);
                ^
Error: Done
    at Object.exports.error ....

@safareli safareli changed the title fix mempry leak in make loop Fix Memory leak in make loop Jun 4, 2018
@felixSchl
Copy link

I think you can eliminate the attempt by holding onto the spawned fiber inside the Bus structure and when killing, kill the fiber first.

@safareli
Copy link
Contributor Author

That's a good idea, it would also make kill block until the fiber is fully cleaned up, tho it means we need to add eff type var to Bus or use unsafeCoerce to make that work.
In PS 0.12 as there is no effect label this could be done safely.

It would be nice to merge this open PRs, and do that later as of PS 0.12 update.

@garyb garyb merged commit 1c273a2 into purescript-contrib:master Nov 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants